Add exporting of results from the context menu#1750
Conversation
There was only a single command for exporting variant analysis results, which would either export the selected result or a given result. From the query history, the command was always calculating the exported result, while we can just give a query ID to export. This will create two separate commands for exporting results, one for exporting the selected results (user-visible) and one for exporting a specific remote query result. This will make it easier to add support for exporting variant analysis results. I'm not sure if there will be impact from renaming the command. I expect the only impact to be that the command history might not show the command in the correct place (i.e. it disappears from recently used commands), but please check if that is the only impact.
This adds the export of variant analysis results. This is unfortunately a larger change than I would have liked because there are many differences in the types and I think further unification of the code might make it less clear and would actually make this code harder to read when the remote queries code is removed. In general, the idea for the export of a variant analysis follows the same process as the export of remote queries, with the difference being that variant analysis results are loaded on-the-fly from dis, rather than only loading from memory. This means it should use less memory, but it also means that the export is slower.
koesie10
force-pushed
the
koesie10/export-results
branch
from
9494da4 to
42c642d
Compare
robertbrignull
left a comment
robertbrignull
left a comment
There was a problem hiding this comment.
I've tried it out and it works for me. I have one question about the code and if we can make class interfaces easier to use, but that's all.
|
|
||
| let result: VariantAnalysisScannedRepositoryResult; | ||
|
|
||
| if (!variantAnalysisManager.areResultsLoaded(variantAnalysis.id, repo.repository.fullName)) { |
There was a problem hiding this comment.
Should this whole if block be a method on VariantAnalysisManager so outside code can see a cleaner interface and doesn't need to know it has to check areResultsLoaded first?
There was a problem hiding this comment.
👍, I've added a new options argument to the loadResults method, so all of this is now contained in the variant analysis results manager.
This adds a new options argument to the `loadResults` method which allows the caller to specify that the results should not be saved to the cache. This exposes a smaller API surface and makes it harder to misuse the methods.
| throw new Error('No variant analysis results currently open. To open results, click an item in the query history view.'); | ||
| } | ||
|
|
||
| if (!queryHistoryItem.completed) { |
There was a problem hiding this comment.
Don't we still want to perform this check for remote queries?
There was a problem hiding this comment.
Yes, we do still need it for the remote queries. However, I felt it was redundant to do it twice, so this is now only checked in the command we call from this method. This should result in the same behaviour for the user.
| await commands.executeCommand('codeQL.exportRemoteQueryResults', finalSingleItem.queryId); | ||
| } else if (finalSingleItem.t === 'variant-analysis') { | ||
| await commands.executeCommand('codeQL.exportVariantAnalysisResults', finalSingleItem.variantAnalysis.id); | ||
| } |
There was a problem hiding this comment.
Is there room for a quick unit test like we have for handleCancel?
There was a problem hiding this comment.
Thanks for the pointer! I've added unit tests for both this method and the handleCopyRepoList which does basically the same thing.
3 participants
This will add exporting of results from the context menu for variant analyses.
Please review this commit-by-commit: the first commit is in preparation for the second commit. Also see the commit messages for more context on these changes.
Checklist
ready-for-doc-reviewlabel there.